Size Change: -241 B (0%)

Total Size: 1.43 MB

_{compressed-size-action}

Ideally, the "save metaboxes" action should hook into the savePost and autosave actions and be awaited there. That would replace the current listener that tries to catch the "save finished" event by checking isSavingPost transitions from true to false.

Do you think we can add an unstable action (action in the sense of @wordpress/hooks action in the save action of the editor package or something like that to address that?

Do you think we can add an unstable action

Yes, I will work on that 👍 It's quite a detour from the original React 18 migration, but I believe it will improve the post save logic a lot.

We'll probably also need filters for the "is autosaveable?" and "is dirty?" values. Now it's implemented with special like forceIsAutosaveable and forceIsDirty props, which have a hasActiveMetaboxes value.

We'll probably also need filters for the "is autosaveable?" and "is dirty?" values. Now it's implemented with special like forceIsAutosaveable and forceIsDirty props, which have a hasActiveMetaboxes value.

Yeah I guess, even if I don't like it much 😬 (a filter in a selector might not be the greatest idea as it doesn't retrigger the subscription on change). My ultimate goal is to actually remove the "editor" package entirely as now, it just became a subset of edit-post and this would remove these hacks but let's leave that for a far future.

Hi @youknowriad 👋 I implemented the editor.SavePost hook in this PR and migrated the requestMetaBoxUpdates call to use it.

I was able to remove all the forceIsSaving and forcePreviewLink props, because the isSavingPost selector now returns true during the entire save process, including hooks, not only during the entity save. No additional information is needed.

Removing the remaining forceIsAutosaveable and forceIsDirty will be harder, I left them unchanged for now.

I'm not sure how to name the editor.SavePost hook. Maybe editor.__unstableSavePost? Also I don't know which parameters are the right one. Currently there is the previous promise, allowing us to chain hook callback asynchronously after each other and to jump out of the hook chain in case of error. And the options object.

By the way, to test the metaboxes I installed the ACF plugin, and noticed they override the editor.savePost action to add some custom validation code:

That would be a good use case for an editor.preSavePost hook, one that runs before the entity save. Also, their code has a little bug where the .catch at the end swallows errors from the original savePost action, instead of letting them propagate. I'm wondering what's the best way to report that to the plugin authors?

@jsnajdr Hey! One of the ACF Devs here. Happened to stumble onto this as part of our report in #44716. Can we just drop our .catch entirely here? I'll dig deeper into this and get it raised as a bug for us to fix.

Can we just drop our .catch entirely here?

Hi @lgladdy 👋 It's great that we connected so quickly 🙂 Some catch handler will still need to be there, but placed slightly differently:

editor.savePost = function() {
  return acf.validateFormPromise().then(
    () => {
      return originalSavePost();
    },
    ( err ) => {
      // ignore validation error
    }
  );
}

This code has the following behavior:

• if the form validation succeeds, original savePost will be called, and the new savePost will return its result promise without any handling: if there is an error thrown from original savePost, it will be passed to Gutenberg to handle. The current ACF code fails to pass the error: it will swallow it.
• if the form validation fails, we're catching and ignoring the validation error. But only the validation error, not an original savePost error! The editor.savePost will return success. We want to do that because acf.validateFormPromise does its own error handling, calling createErrorNotice, resetting the status... If we didn't swallow the validation error, it would be passed to Gutenberg code which would either show a second error notice or not handle the error at all, causing a runtime error.

I fixed some unit tests:

• the isSavingPost started looking at the editor store internal flag rather than the entity's "is saving" flag, because we're now saving more than just the entity. The isSavingPost and isEditedPostAutosaveable tests needed to be updated.
• the PostPreviewButton unit tests needed to be changed because we started to use useSelect internally, and newly we need to mock the return values of selectors.

Everything is green now and ready for final review. The main thing we need to decide on is the exact name of the hook and its parameters.

Anything missing here? This looks good in my testing.

As far as I know nothing is missing and once the tests come back green, the PR can be merged.

A very relevant e2e test is failing right now:

PostPublishButton › should be disabled when metabox is being saved

I'll need to address that before merging.

Looks like this fell through the cracks, I thought it was a nice refactor :) and might help in fixing some preview bugs. Should we try to update this, what was the blocker?

Should we try to update this, what was the blocker?

There is one failing e2e test that relies on the details of the old implementation, and is hard to update. We'll probably have to remove the test.

I didn't forget about this PR, I still think about it every few days 🙂

@jsnajdr, I created Playwright migration PR for PostPublishButton e2e tests - #52285. Let me know if I can help with failures related to this PR.

I rebased this old PR onto latest trunk, resolved the conflicts, and removed the "button should be disabled when metabox is being saved" e2e test.

Saving metaboxes, i.e., the requestMetaBoxUpdates action, is now an integral part of the save process, registered as a plugin-like filter. Before this PR, there were actually two semi-parallel save processes, and the PostPreviewButton had to figure out on its own, by evaluating condition like isSaving = isPostSaving || isMetaBoxSaving, whether it should be disabled.

The e2e test was relying on state that is now too internal to be reliably observable by a e2e test.

If the CI checks pass, I believe this can finally be merged.

If this makes its way to 6.3, it will need a dev note about the new editor.__unstableSavePost hook. But it probably won't, 6.3 is in a very late stage of the beta cycle. Also, the hook is named __unstable. We should test it with plugins like ACF before stabilizing it.

@jsnajdr, should we add basic documentation for the new hook somewhere? I think that would also help the stabilization of it.

P.S. These two come to mind regarding third-party implementations - #13413 and #49811.

should we add basic documentation for the new hook somewhere?

Yes, I'll document it. The best place seems to be the docs/reference-guides/filters folder.

These two come to mind regarding third-party implementations

I've been chatting with @lgladdy about using the new hook (and adding some new ones, like a BeforeSavePost one) in ACF. That should help figure out what the right inputs and outputs are for the hook, which is the main open question that needs to be answered before stabilizing it.

Thank youyou, @jsnajdr!

The discussions could also serve for sharing initial documentation and gathering feedback.

Cc-ing @sc0ttkclark, just in case they're also interested in this discussion.

If you get docs or have any discussions about this, I'd love to be a part of that.

I see a few ACF references here, is there a working prototype example of how to leverage what's in this PR beyond documenting it which might take longer? I assume the ACF example will cover the use-cases I'm after as I'm doing something similar for Pods already.

(reposting on my main account, sorry for the confusion)

I see a few ACF references here, is there a working prototype example of how to leverage what's in this PR beyond documenting it which might take longer?

We discussed the specific ACF use case with @lgladdy some time ago, the discussion starts here:
#44971 (comment)

In short, Gutenberg should add also a new preSavePost hook, in addition to the current one which runs after save, and then ACF can use it to run its validation code. Currently, ACF monkey-patches the savePost action, replacing it with a wrapper that first does the ACF validation and then calls the original savePost version.

Hi @lgladdy 👋 It's great that we connected so quickly 🙂 Some catch handler will still need to be there, but placed slightly differently:

@jsnajdr Just to note, we shipped a fix for this in ACF 6.2.1 tonight, so hopefully debugging savePost issues with ACF installed will be easier now! Sorry this took so long!

Pre save hook where we could do what ACF seeks to do for validation would be very helpful for other plugins too.

Following up here, did a doc get posted on how to utilize this yet?

Hey @sc0ttkclark I was following this thread as well and after testing the release, it doesn't solve the issue. I believe they only added filter because they needed it for metabox and not for plugins validation consideration.

The filter was added to LINE 190 after the post has already been saved on line 157

So, even though it trigger an error if the filter third party plugin validation failed, it doesn't stop the posts from been published or updated.

It's unfortunate we have to keep waiting for this.

In short, Gutenberg should add also a new preSavePost hook, in addition to the current one which runs after save, and then ACF can use it to run its validation code. Currently, ACF monkey-patches the savePost action, replacing it with a wrapper that first does the ACF validation and then calls the original savePost version.

I tried adding that here (feedback welcome!): #58022